fix(admin): add proper validation for campaign fee override (#531) - #733
Open
thebabalola wants to merge 1 commit into
Open
fix(admin): add proper validation for campaign fee override (#531)#733thebabalola wants to merge 1 commit into
thebabalola wants to merge 1 commit into
Conversation
Fixes validation bug where per-campaign fee overrides above 10000 basis points (100%) were not properly rejected. Changes: - Add InvalidFeeOverride error type to src/errors.rs - Replace ValidationFailed with InvalidFeeOverride in set_campaign_fee_override - Strengthen validation to reject fee_bps > PLATFORM_FEE_ABSOLUTE_MAX_BPS (10000) - Update test to expect InvalidFeeOverride instead of ValidationFailed - Add test for edge case: exactly 10000 bps should be rejected Fixes issue Iris-IV#531: set_campaign_fee_override: per-campaign fee set to 10001+ basis points is not validated on input
|
@thebabalola Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Author
|
hey, fixed the fee override validation bug for #531. added a dedicated InvalidFeeOverride error type and strengthened the validation to properly reject fees above 100% (10,000 bps). ready for review! |
davidmaronio
requested changes
Aug 3, 2026
davidmaronio
left a comment
davidmaronio
left a comment
Contributor
There was a problem hiding this comment.
the dedicated InvalidFeeOverride error is a reasonable ergonomics improvement, and the errors.rs wiring (variant plus name() arm) is done correctly, which several sibling prs got wrong. but the framing and a few details need work:
- the pr description says it "fixes a validation bug where overrides above 10,000 bps were not properly validated", but both bounds checks already existed on main and already rejected those values with
ValidationFailed. this pr only changes which error code comes back. that's fine as a change, but the description and changelog should say "clearer error" not "fixes validation bug", and it's worth confirming that's enough to close #531. - src/admin.rs:188-196 - the first check (
> PLATFORM_FEE_ABSOLUTE_MAX_BPS, 10000) is now dead code: anything above 10000 is also above 1000, so the second check would catch it anyway even alone. since both now return the same error, collapse to a singleif fee_bps > crate::PLATFORM_FEE_MAX_BPScheck, or return distinct errors if the two tiers matter. - src/tests/test_admin.rs:705-707 - the "edge case: exactly 10000" test doesn't exercise the absolute-max path; 10000 is rejected by the 1000-bps platform max like any other large value. the comment implies it's testing the 100% boundary but it isn't.
- changing an established error code from
ValidationFailedto a new variant is client-visible. worth a changelog entry flagging it for anyone matching on error codes. - discriminant 46 is also claimed by #728 (
InvalidCommentId) and #731 (DuplicateCampaignTitle). whoever lands first wins; expect to renumber on rebase. - CI is red because this branch keeps the broken orphaned
ProofOfHeartContractblock in src/admin.rs (this pr even reorders itsuselines rather than deleting it). that block doesn't compile on any branch; rebase once the cleanup pr lands.
gate: branch is DIRTY and CI is red, so resolve conflicts, rebase onto main after the orphan-code cleanup lands, and re-run CI to green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #531
Summary of Changes
Fixes validation bug where per-campaign fee overrides above 10,000 basis points (100%) were not properly validated on input.
What Changed
Error::InvalidFeeOverridetosrc/errors.rs(46th variant)set_campaign_fee_overrideinsrc/admin.rsto:PLATFORM_FEE_ABSOLUTE_MAX_BPS(10,000) → returnsInvalidFeeOverridePLATFORM_FEE_MAX_BPS(1,000) → returnsInvalidFeeOverridetest_campaign_fee_override_above_max_rejectednow expectsInvalidFeeOverrideinstead of generalValidationFailedKey Design Decisions
InvalidFeeOverrideerror instead of the genericValidationFailedmakes it clear why the validation failed in logs and event payloadsPLATFORM_FEE_ABSOLUTE_MAX_BPS= 10,000) prevents nonsensical >100% feesPLATFORM_FEE_MAX_BPS= 1,000) enforces business policyTesting / Local Verification
cargo test test_campaign_fee_override_above_max_rejectedExpected result: All tests pass with the new error validation
Security Impact
This fix prevents a scenario where an admin could set campaign fees above 100%, which would result in donors paying more than they intended. The validation ensures fees are always within reasonable bounds.